Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

small restructuring of the routines for the AHC calculation with multiple Fermi energies and adaptive refinement #289

Merged
merged 12 commits into from
Feb 10, 2020

Conversation

hjunlee
Copy link
Contributor

@hjunlee hjunlee commented Sep 1, 2019

Dear all:

This commit modifies the routines in berry.F90 so as to remove unnecessary multiple calls of berry_get_imf_klist in the berry module for calculations of AHC with adaptive refinement turned on and multiple Fermi energies (nfermi>1).
The current structure results in the duplication of some calculations unnecessarily.

With this commit, we can achieve much speed-up, in particular, in the case of large nfermi and rather dense adaptive-refinement k-mesh.

I made an effort to change the current structure of the berry module as little as possible even though I can achieve more speed-up with more restructuring.

Hyungjun Lee

@hjunlee hjunlee closed this Sep 1, 2019
@hjunlee hjunlee reopened this Sep 1, 2019
@hjunlee hjunlee closed this Sep 1, 2019
@hjunlee hjunlee reopened this Sep 1, 2019
@codecov
Copy link

codecov bot commented Sep 1, 2019

Codecov Report

Merging #289 into develop will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #289      +/-   ##
===========================================
+ Coverage    65.60%   65.78%   +0.17%     
===========================================
  Files           29       29              
  Lines        17939    17957      +18     
===========================================
+ Hits         11769    11813      +44     
+ Misses        6170     6144      -26     
Impacted Files Coverage Δ
src/parameters.F90 82.94% <0.00%> (+0.08%) ⬆️
src/postw90/berry.F90 76.27% <0.00%> (+2.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0d5e4f...35a1c51. Read the comment docs.

@hjunlee
Copy link
Contributor Author

hjunlee commented Sep 12, 2019

I don't understand why this minimal modifications lead to the failure in the check of codecov/patch.
How can I pass this check?

Change of the length of the character variable of file_name in order to avoid the truncation.
@giovannipizzi
Copy link
Member

Don't worry, those tests are not critical.
Anyway, the reason is that more than 50% of the lines you added are untested.
Would you mind adding a test that goes through the new code path? Thanks!

@giovannipizzi
Copy link
Member

Hi @hjunlee - since we are planning to release v3.1 in about 1 week, if you would like to see this merged, it would be great if you could add the tests.
Do you think you would have the time to do it?
Here are the instructions if you never did it. If you have questions just let us know.
Thanks in advance!

@hjunlee
Copy link
Contributor Author

hjunlee commented Feb 9, 2020

Dear Giovanni:

I added the test for calculation of anomalous Hall conductivity for bcc Fe with adaptive mesh refinement and Fermi energy scan.

Benchmark data comes from the calculation using the most recent develop version without my modification. Since my modification just removes unnecessary multiple calls, the results should be same and this is confirmed.

Sincerely,

Hyungjun Lee

@giovannipizzi
Copy link
Member

Thanks a lot!

@giovannipizzi giovannipizzi merged commit 3895b6f into wannier-developers:develop Feb 10, 2020
manxkim pushed a commit to manxkim/wannier90 that referenced this pull request Jan 10, 2021
small restructuring of the routines for the AHC calculation with multiple Fermi energies and adaptive refinement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants